Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Replace UPDATE with UPSERT on device_max_stream_id table #6363

Merged
merged 5 commits into from
Nov 15, 2019

Conversation

anoadragon453
Copy link
Member

Helps address #6311 (for the device_max_stream_id table anyways).

We had a problem where when our multiple schema delta files were combined into a single schema, it left out an INSERT statement that insert the first row into the device_max_stream_id. Synapse assumes this row exists and runs an UPDATE against it. However, this would do nothing if no row existed, which was the case here.

This change changes the UPDATE to an UPSERT, by adding a new column to the table with a unique constraint, and always trying to insert with the same value. If a conflict arises (which is should do any time there's a row in that table), then the stream_id value is updated. If there's no row in there (which will be the case with homeservers initially) then a simple INSERT occurs.

There may be a more elegant way to do this, but this works.

@anoadragon453 anoadragon453 self-assigned this Nov 14, 2019
@anoadragon453 anoadragon453 changed the title upsert on max to_device stream id Replace UPDATE with UPSERT on device_max_stream_id table Nov 14, 2019
@anoadragon453 anoadragon453 requested a review from a team November 14, 2019 17:20
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so with this I think it will still reset after it's restarted with this new code, but then update correctly after that, so we swill still get one more round of UISIs after the update, after which it will be fine. We could add some code to the StreamIdGenerator where it loads the current ID to work around this... not sure if it's worthwhile though - probably better to just get this deployed.

sql = "UPDATE device_max_stream_id SET stream_id = ?"
else:
# No rows, perform an insert
sql = "INSERT INTO device_max_stream_id (stream_id) VALUES (?)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also do the update as before and then check the affected row count which would then just be a single query in the normal case, but if this is how we're doing upserts elsewhere then I guess just stick with that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little more complicated since we have to check that the stream_id we're inserting is greater than what's currently in the DB. If it's not, then the UPDATE will return 0 rows affected.

We don't want to then insert another row at that point.

@anoadragon453
Copy link
Member Author

I'll merge this as-is and we can improve in the future if necessary.

@anoadragon453 anoadragon453 merged commit 657d614 into develop Nov 15, 2019
@anoadragon453 anoadragon453 deleted the anoa/device_sql branch November 15, 2019 14:02
@anoadragon453 anoadragon453 restored the anoa/device_sql branch November 15, 2019 14:04
@anoadragon453 anoadragon453 deleted the anoa/device_sql branch November 15, 2019 14:05
anoadragon453 added a commit that referenced this pull request Dec 16, 2019
babolivier pushed a commit that referenced this pull request Sep 1, 2021
* commit '657d614f6':
  Replace UPDATE with UPSERT on device_max_stream_id table (#6363)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants